Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add new Rails/ActiveRecordCalculation cop #1371

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

fatkodima
Copy link
Contributor

Closes #1205.

In my app I found 11 offences of this cop.

@fatkodima fatkodima force-pushed the new-active_record_calculation-cop branch from ce696f1 to 8223b8d Compare September 22, 2024 18:15
module RuboCop
module Cop
module Rails
# Enforces the use of ActiveRecord calculation methods instead of `pluck`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add the rationale here: it avoids loading potentially many values into memory by doing the calculations through the equivalent database function.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea 👍

@fatkodima fatkodima force-pushed the new-active_record_calculation-cop branch from 8223b8d to 0d2113a Compare September 22, 2024 21:05
Rails/ActiveRecordCalculation:
Description: 'Use ActiveRecord calculation methods instead of `pluck` followed by Enumerable methods.'
Enabled: pending
VersionAdded: '<<next>>'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would need to be marked unsafe because Ruby's sum/max aren't always compatible with the DB's:

User.pluck(:name).sum # "AliceBobCathy"
User.sum(:name) # PG::UndefinedFunction: ERROR:  function sum(character varying) does not exist

# User.pluck(:email).max { |email| email.length }
# User.pluck(:email).max(2)
# User.pluck(:id, :company_id).max
# User.pluck(:age).count
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is kinda a weird counterexample, because User.count(:age) would actually be more efficient and falls in line with this cop's premise. 🤔

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cop idea: prefer .maximum and .minimum over .pluck & .max or .min
3 participants